Skip to content

feat: add an in-process worker transport (no gRPC)#548

Open
mdashti wants to merge 3 commits into
datafusion-contrib:mainfrom
paradedb:moe/protocol-grpc-shm
Open

feat: add an in-process worker transport (no gRPC)#548
mdashti wants to merge 3 commits into
datafusion-contrib:mainfrom
paradedb:moe/protocol-grpc-shm

Conversation

@mdashti

@mdashti mdashti commented Jul 10, 2026 •
edited
Loading

Copy link
Copy Markdown

What

This PR makes the WorkerChannel abstraction usable by a transport that is not gRPC, and adds the in-process reference transport that proves it.

Why

#512 abstracted the worker protocol, but gRPC stayed the only implementation and the proto sat behind the grpc feature, so nothing built or ran without it.

How

One commit per concern, each with its own test:

  • Moved the prost types out from behind grpc. Only the tonic client and server stay gated, and the generator emits those gates.
  • Decoupled the benchmarks dev-dependency (it re-unified grpc into every test build) and added a unit-test-no-grpc CI job that runs the whole lib suite with the feature off.
  • InProcessChannelResolver: routes the three protocol methods to a co-located Worker. No gRPC, no IPC, no serialization round trip.
  • metrics_store() with a public insert, and a no-gRPC decode_task_metrics: a driver whose transport returns metrics out-of-band files the decoded frames into the store the EXPLAIN rewrite reads. Happy to break this one out into its own PR too if preferred.

Tests

The in-process transport runs a distributed GROUP BY end to end with grpc off in CI, and a unit test pins the metrics frame-to-store path.

mdashti and others added 3 commits July 9, 2026 17:06
@mdashti @stuhood
Moved the worker-protocol messages out from behind the grpc feature.
72f139d 
The prost message types carry no tonic dependency, so a transport that is
not gRPC can speak the same wire shape without pulling in the gRPC stack.
Only the tonic client and server stay gated; the generator emits those
gates so a regeneration cannot drop them. tonic-prost feeds only the
generated client and server, so it moves behind the feature too.

Co-authored-by: Stu Hood <stuhood@gmail.com>
@mdashti @stuhood
Made the no-gRPC build compile and its unit suite run in CI.
703113b 
The benchmarks crate's dev-dependency on the lib re-unified grpc into
every test build, so a genuine no-gRPC test run was impossible; the
dataset suites move into the benchmarks crate and the gRPC-coupled test
utilities gate behind grpc. A unit-test-no-grpc job then runs the whole
lib suite with the feature off.

Co-authored-by: Stu Hood <stuhood@gmail.com>
@mdashti @stuhood
Added an in-process worker transport that needs no gRPC.
36af89b 
InProcessChannelResolver routes the three protocol methods straight to a
co-located Worker, with no gRPC, no IPC, and no serialization round-trip:
the reference implementation of the protocol for a co-located worker, and
the first transport that exercises the abstraction with grpc off. Its
end-to-end test (a distributed GROUP BY across tasks) runs under the
no-gRPC CI job.

Co-authored-by: Stu Hood <stuhood@gmail.com>
@mdashti mdashti marked this pull request as draft July 10, 2026 00:40
stuhood

This comment was marked as outdated.

Sign in to view

@mdashti mdashti force-pushed the moe/protocol-grpc-shm branch from be44b9c to 0dfe291 Compare July 10, 2026 03:24
@mdashti mdashti mentioned this pull request Jul 10, 2026
Open
@mdashti

mdashti commented Jul 10, 2026

Copy link
Copy Markdown
Author

Thanks @stuhood. Pulled dispatch_plan_source.rs, the query_coordinator.rs hook, and the network_boundary.rs routing out of this PR; they'll come as a follow-up once this lands. The tip is now the protocol un-gate, the no-gRPC CI job, the in-process transport, and the metrics surface.

@mdashti mdashti marked this pull request as ready for review July 10, 2026 03:33
@mdashti

mdashti commented Jul 10, 2026

Copy link
Copy Markdown
Author

@gabotechs Can you please take a look?

gabotechs
gabotechs reviewed Jul 10, 2026
View reviewed changes

@gabotechs gabotechs left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before reviewing, I see this PR contains a lot of changes that are unnecessary, for example:

  • Tweaks to the CI. I'd leave the CI as it is for now, that can be evaluated separately
  • Changes to the public API, like exposes the metrics_store method in DistributedExec
  • Move some integration tests to the benchmarks

Before reviewing, I'd recommend to really scope the PR just to have an in-memory implementation of the WorkerChannel.

Also, as I don't think there's any external use-case, it might be better to delay adding more WorkerChannel implementations to when they are needed. The WorkerChannel abstraction is in early stages, and it will likely suffer several breaking changes in the short term as we try to accommodate more features. Having more implementations that this project is not going to use will mean paying the price of maintaining those implementations with no benefit, so I'd recommend to defer this work a bit until there's a valid use case.

For example, when we have AQE re-optimizing queries on the fly, there'll be a use case why people would want to run this project in a fully in-memory fashion, once we get that it might be a good time to start thinking about officially supporting more implementations in this project.

@stuhood

stuhood commented Jul 10, 2026

Copy link
Copy Markdown

Before reviewing, I'd recommend to really scope the PR just to have an in-memory implementation of the WorkerChannel.

I agree: I'll attempt to scope this down to only adding the new WorkerChannel abstraction. But I do think that that will require one additional CI job that runs unit tests with --no-default-features, as that's the only way to prove that things are actually buildable.

Also, as I don't think there's any external use-case, it might be better to delay adding more WorkerChannel implementations to when they are needed. The WorkerChannel abstraction is in early stages, and it will likely suffer several breaking changes in the short term as we try to accommodate more features. Having more implementations that this project is not going to use will mean paying the price of maintaining those implementations with no benefit, so I'd recommend to defer this work a bit until there's a valid use case.

I think that having two implementations of trait WorkerChannel in the repo is a good thing: any trait with exactly one (nearby) implementation seems much more likely to become unusable by alternative implementations. And the second implementation in this case is ~230 lines (plus 1 CI job)... hopefully not much of a maintenance burden.

@stuhood stuhood force-pushed the moe/protocol-grpc-shm branch from 0dfe291 to 36af89b Compare July 10, 2026 23:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gabotechs gabotechs gabotechs left review comments
+1 more reviewer
@stuhood stuhood stuhood approved these changes
Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mdashti @stuhood @gabotechs